refactor sync session to use a queue for extension file save events#4211
refactor sync session to use a queue for extension file save events#4211
Conversation
🦋 Changeset detectedLatest commit: dba3f32 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR refactors the SessionManager to use a queue-based approach for handling file save events, replacing the previous state-based implementation. The key improvement is better handling of asynchronous file save events from the extension through event queuing and reliance on taskId <-> sessionId mapping.
Key Changes:
- Replaced state-based file path tracking with an event queue that stores file update events with timestamps
- Removed the
destroy()method and associated cleanup calls, simplifying session lifecycle management - Consolidated session state management using per-task tracking (
taskGitUrls,taskGitHashes,sessionTitles)
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
SessionManager.ts |
Core refactor: replaced state-based tracking with queue-based event processing, changed setPath() to handleFileUpdate(), removed destroy() method, improved sync logic to process multiple tasks per sync cycle |
SessionClient.ts |
Moved KILO_PLATFORM environment variable override from manager to client's create() method; fixed URL construction bug using proper URL constructor |
session-manager-utils.ts |
Removed kilo_destroySessionManager() utility function as destroy() no longer exists |
ClineProvider.ts |
Updated to use handleFileUpdate() instead of setPath(); removed destroy() calls on task focus and disposal |
cli/src/state/atoms/effects.ts |
Updated message handlers to call handleFileUpdate() instead of setPath() |
cli/src/commands/new.ts |
Removed session cleanup logic using destroy() as it's no longer needed |
cli/src/cli.ts |
Removed destroy() call during CLI disposal |
| Test files | Split tests into two focused test suites: general SessionManager tests and specific syncSession tests with comprehensive race condition coverage |
cli/src/commands/__tests__/new.test.ts |
Updated to reflect removal of session destroy functionality |
.changeset/common-buckets-tickle.md |
Added changeset for patch release |
src/shared/kilocode/cli-sessions/core/__tests__/SessionManager.spec.ts
Outdated
Show resolved
Hide resolved
src/shared/kilocode/cli-sessions/core/__tests__/SessionManager.spec.ts
Outdated
Show resolved
Hide resolved
RSO
left a comment
RSO
left a comment
There was a problem hiding this comment.
It's hard to piece together all the different changes, but nothing stood out to me so ✅
GH diff view doesn't do it justice. |
pandemicsyn
left a comment
pandemicsyn
left a comment
There was a problem hiding this comment.
one question inline around whether we need to worry about losing stuff during tear down, but lgtm!
| try { | ||
| logs.info("Disposing Kilo Code CLI...", "CLI") | ||
|
|
||
| await this.sessionService?.destroy() |
There was a problem hiding this comment.
Not blocker, not even sure its really an issue, but do we need to delay exiting on SIGINT to give the background sync stuff a chance to trigger?
Wondering if we'd have stuff in the session manager queue that we're losing.
There was a problem hiding this comment.
I think we do, I'll add back the destroy method
this was caused by a previous race condition
useful if someone has their node_modules in the diff
3 participants
Context
Improves
SessionManagerto better handle the asynchronicity of file save events from the extension.Implementation
Instead of maintaining state, preserve events in a queue and process on a timer. Rely on
taskId <-> sessionIdfor consistency.